exec: do not leak session IDs on errors #20394
Conversation
openshift-ci
bot
added
release-note
approved
libpod/container_exec.go
Outdated
| @@ -759,11 +759,21 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi | |||
| // Exec emulates the old Libpod exec API, providing a single call to create, | |||
| // run, and remove an exec session. Returns exit code and error. Exit code is | |||
| // not guaranteed to be set sanely if error is not nil. | |||
| func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resizeChan <-chan resize.TerminalSize, isHealthcheck bool) (int, error) { | |||
| func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resizeChan <-chan resize.TerminalSize, isHealthcheck bool) (ExitCode int, Err error) { | |||
There was a problem hiding this comment.
nit local vars should IMO always start lower case.
giuseppe
force-pushed
the
cleanup-exec-session-on-errors
branch
from
2105e26 to
266b0d3
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
giuseppe
force-pushed
the
cleanup-exec-session-on-errors
branch
from
266b0d3 to
9b99b8d
Compare
Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe
force-pushed
the
cleanup-exec-session-on-errors
branch
from
9b99b8d to
bb8abdc
Compare
always cleanup the exec session when the command specified to the "exec" is not found. Closes: containers#20392 Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe
force-pushed
the
cleanup-exec-session-on-errors
branch
from
bb8abdc to
fa19e1b
Compare
|
/lgtm |
|
This has broken CI. All remote system tests are now failing on (what looks like) all PRs. @giuseppe please treat this as urgent. |
|
not even sure how to fix it properly. @mheon should we add a new function to the bindings to allow ExecRemove from the remote client? I don't think the cleanup should be done automatically server-side on failures |
|
I'm really confused: how did this pass CI? It's failing hard, solid not-flaking 100%, on every other in-flight PR that I see. |
|
ideally, a merge should be possible only when the CI is all green (or has a way to explicitly force it if needed). A |
|
Oh! That's it, I see now. It merged while CI was still running. I hadn't seen that. Yes, "ideally". Nobody anywhere in the universe will disagree with you. Unfortunately this is not an ideal world [citation needed]. That's why you see so many slash-lgtm-slash-hold: because slash-lgtm, while CI is running, is catastrophic. Thank you for noticing that that's what happened. I feel better now that I understand. |
Followup to containers#20394. For years (since BATS 1.5) we've been seeing and ignoring nasty red warnings at the end of every system test run. Thanks for fixing it, @giuseppe! But it broke down in the '?' case when $expected_rc is empty: test/system/helpers.bash: line 345: [: -eq: unary operator expected Simple fix. Signed-off-by: Ed Santiago <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
always cleanup the exec session when the command specified to the "exec" is not found.
Closes: #20392
Does this PR introduce a user-facing change?